-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Show all items when manually opening ComboBox menu #1749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…fied by filter operation
… show full menu behavior
…dits to textfield changed show all menu items tracker to useState instead of ref
also fixing logic of when to open combobox if default items is empty vs controlled items + the filtered list provided is empty but user is updating with a unfiltered list via onMenuOpenManual
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
TODO: write docs example for how someone would go about changing controlled |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| }; | ||
|
|
||
| let triggerState = useMenuTriggerState({...props, onOpenChange}); | ||
| let open = (focusStrategy?: FocusStrategy, showAllItems?: boolean, trigger?: MenuTriggerAction) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both showAllItems and trigger here? Could trigger === 'manual' be used instead to determine whether to show all items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figureed I would include both so that consumers of useComboBoxState wouldn't be forced into the "manual or focus trigger = show all items" logic, felt like this gives them more freedom to define what would get displayed and when for their own ComboBox implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm not sure I follow. you mean if someone called comboBoxState.open() manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, or if someone wrote their own useComboBox but used our useComboBoxState and wanted their combobox not to show all items when opening the menu. They could then have something like this in their custom useComboBox hook:
let onKeyDown = (e: KeyboardEvent) => {
switch (e.key) {
case 'ArrowDown':
state.open('first', false, 'manual');
meaning their default combobox behavior would be to only show the filtered collection at all times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think this may be providing too much flexibility. Seems like in that case maybe it would be preferable to add a prop to control the behavior for everyone equally. I guess if we decide to do that it would also solve this. For now, I think keeping the API simple is preferable.
| shouldCloseOnBlur = true | ||
| } = props; | ||
|
|
||
| let [showAllItemsTracker, setShowAllItemsTracker] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it a "tracker"?
| let [showAllItemsTracker, setShowAllItemsTracker] = useState(false); | |
| let [showAllItems, setShowAllItems] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different name here because the open and toggle functions have showAllItems as a param. Maybe I could rename this to displayAllItems? I definitely could change this to showAllItems if I remove that parameter, but that's subject to the discussion in a previous comment
| let menuOpenTrigger = useRef('focus' as MenuTriggerAction); | ||
| let onOpenChange = (open: boolean) => { | ||
| if (props.onOpenChange) { | ||
| props.onOpenChange(open, open ? menuOpenTrigger.current : undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a little hacky to use a ref for this... I guess the other option would be to not use menu trigger state at all and reimplement it and that seems possibly worse 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah def a bit hacky, but as you said the alternative would be to reimplement the open state logic from useMenuTriggerState+useOverlayTriggerState just so we can get onOpenChange to return the menu trigger action as well. I figured changing those hooks themselves to track menu trigger action was undesirable as well.
as per review comment
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
Closes #1705
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: